Add Tree concurrency tests #186

7hong13 · 2024-05-27T04:24:55Z

What this PR does / why we need it?

Any background context you want to provide?

What are the relevant tickets?

Add Tree concurrency tests yorkie-js-sdk#792

Fixes #

Checklist

Added relevant tests or not required
Didn't break anything

Summary by CodeRabbit

New Features
- Enhanced JSON tree editing capabilities with new methods.
Bug Fixes
- Updated OkHttpClient protocol to improve connection stability.
- Improved document status management for better synchronization.
Improvements
- Upgraded androidxLifecycle and androidx-annotation dependencies for enhanced performance and stability.
- Refactored test functions for improved readability and maintenance.
- Consolidated internal test annotations for streamlined testing.
Chores
- Updated CI workflow configuration to use specific test classes.

coderabbitai · 2024-05-27T04:25:01Z

Walkthrough

The recent updates introduce several key modifications: CI workflow adjustments to specify test classes, dependency version upgrades, consolidation of test annotations, and enhancements to JSON tree concurrency testing. Additionally, document status management in the Client class now uses a setStatus method, and synchronization logic has been refined. These changes aim to improve test specificity, code maintainability, and overall functionality.

Changes

File Path	Change Summary
`.github/workflows/ci.yml`	Replaced test annotations with specific test classes for `JsonTreeTest`, `JsonTreeSplitMergeTest`, and `JsonTreeConcurrencyTest`.
`gradle/libs.versions.toml`	Updated `androidxLifecycle` to version "2.8.0" and `androidx-annotation` to version "1.8.0".
`yorkie/src/androidTest/kotlin/dev/yorkie/TestAnnotations.kt`	Consolidated multiple internal annotations into a single internal annotation `TreeTest`.
`yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt`	Updated protocol in OkHttpClient builder from `HTTP_1_1` to `H2_PRIOR_KNOWLEDGE`.
`.../JsonTreeConcurrencyTest.kt`	Added `companion object` and modified `runTestConcurrency` function with additional parameters and `suspend` modifier.
`.../JsonTreeConcurrencyTestHelpers.kt`	Added internal types, interfaces, enums, functions, and data classes for concurrency testing on JSON trees.
`.../JsonTreeSplitMergeTest.kt`	Removed `@TreeSplitMergeTest` annotation and simplified method calls by removing `splitLevel` parameter.
`.../JsonTreeTest.kt`	Removed `@TreeBasicTest` annotation while keeping `@TreeTest`.
`yorkie/src/main/kotlin/dev/yorkie/core/Client.kt`	Updated document status management to use `setStatus` method and refined synchronization logic.
`yorkie/src/main/kotlin/dev/yorkie/document/Document.kt`	Changed `status` setter visibility to `private` and added `setStatus` method.
`yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt`	Added `edit` method with split level parameter and modified existing `edit` method.
`yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt`	Refactored test functions to use `runTest` for common setup and assertions.
`.../DocumentTest.kt`	Refactored test functions to use `runTest` for improved readability and maintenance.
`.../JsonCounterTest.kt`	Refactored test methods to use `runTest` for concise syntax and improved readability.

In code we trust, our tests align,
With trees of JSON, concurrency fine.
Through methods clear and status set,
Our Yorkie runs smooth, no need to fret.
With protocols and versions new,
We march ahead, to skies so blue.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to full the review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

7hong13 · 2024-05-27T09:28:08Z

Tests are failing because some server responses are missing, but it only happens on the local server. The tests work fine on the real server. I'll delve deeper into the failing tests to figure out what's causing this later.

There was an issue with the following situation:

c1.attachAsync(document).await()
c2.attachAsync(document).await() // intermittently, there's a timing issue where the watch stream for c2 isn't set up yet.
c1.updateAsync(document).await() //  it causes c1 to push new changes before c2 is watching the document.

It caused the TCs to keep failing. It is fixed in 286eca6.

coderabbitai

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 055c318 and 9f86bbf.

Files selected for processing (14)

.github/workflows/ci.yml (1 hunks)
gradle/libs.versions.toml (1 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/TestAnnotations.kt (1 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt (1 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTestHelpers.kt (1 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeSplitMergeTest.kt (12 hunks)
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt (2 hunks)
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (2 hunks)
yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (2 hunks)
yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt (1 hunks)
yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt (1 hunks)
yorkie/src/test/kotlin/dev/yorkie/document/DocumentTest.kt (3 hunks)
yorkie/src/test/kotlin/dev/yorkie/document/json/JsonCounterTest.kt (3 hunks)

Files skipped from review due to trivial changes (5)

gradle/libs.versions.toml
yorkie/src/androidTest/kotlin/dev/yorkie/TestAnnotations.kt
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt
yorkie/src/test/kotlin/dev/yorkie/document/DocumentTest.kt
yorkie/src/test/kotlin/dev/yorkie/document/json/JsonCounterTest.kt

Additional comments not posted (13)

yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1)

15-15: Verify server support for HTTP/2 protocol.

.github/workflows/ci.yml (1)

99-101: Updated CI configuration to use class-based test selection.

This change should help in running specific tests more reliably, ensuring that only the relevant tests for each class are executed during CI builds.

yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTestHelpers.kt (1)

1-231: Comprehensive addition of concurrency test helpers.

The introduction of these helpers is crucial for robust concurrency testing of the Tree component. This will significantly aid in preventing concurrency-related issues and ensuring the integrity of operations under concurrent access.

yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt (1)

78-213: Enhanced test coverage for Client operations.

The modifications and additions to the test methods enhance coverage and robustness, ensuring that the Client class behaves as expected across various operations such as activation, deactivation, attachment, and synchronization.

yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeSplitMergeTest.kt (1)

Line range hint 36-231: Robust testing for JsonTree split and merge operations.

The tests cover a wide range of scenarios, including edge cases and concurrent modifications, which are crucial for ensuring the correctness and robustness of the split and merge operations in the JsonTree.

yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt (1)

134-145: Added overloaded edit method with split level support.

The addition of this method enhances the flexibility and functionality of the JsonTree operations, allowing for more granular control over tree modifications, which is essential for certain types of edits.
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt (3)
329-358: Ensure proper client management in concurrency tests.

Verification successful

The function runTestConcurrency in JsonTreeConcurrencyTest.kt correctly manages client activation and deactivation using activateAsync and deactivateAsync.

JsonTreeConcurrencyTest.kt lines 329-358: Proper client management is ensured with activateAsync and deactivateAsync.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify proper client management in concurrency tests.

# Test: Search for proper client activation and deactivation in concurrency tests.
rg --type kotlin $'activateAsync|deactivateAsync'
Length of output: 10534
213-325: Ensure proper synchronization and thread safety in concurrent tests involving style changes.

21-210: Ensure proper synchronization and thread safety in concurrent tests.
yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (2)

454-456: Properly encapsulated method for setting document status.

457-457: Garbage collection method correctly checks conditions before execution.

yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (2)

492-492: Correctly implemented method for setting client status during attachment.

538-538: Properly implemented method for setting client status during detachment.

coderabbitai

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9f86bbf and 286eca6.

Files selected for processing (1)

yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (3 hunks)

Files skipped from review as they are similar to previous changes (1)

yorkie/src/main/kotlin/dev/yorkie/core/Client.kt

skhugh · 2024-05-28T03:21:11Z

yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt

@@ -12,7 +12,7 @@ const val GENERAL_TIMEOUT = 3_000L
 fun createClient() = Client(
    "http://10.0.2.2:8080",
    unaryClient = OkHttpClient.Builder()
-        .protocols(listOf(Protocol.HTTP_1_1))
+        .protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE))


we should stay on http 1 as http 2 is very unstable

using http2 is outdated code. I just tested it temporarily.

skhugh · 2024-05-28T03:29:18Z

yorkie/src/main/kotlin/dev/yorkie/document/Document.kt

@@ -451,6 +451,10 @@ public class Document(
        onlineClients.value -= actorID
    }

+    internal fun setStatus(newStatus: DocumentStatus) {


do we need additional setter method for this?
Kotlin's properties are different from Java's fields, and it is not wrong to use properties for simple getter/setter's.
(https://blog.kotlin-academy.com/kotlin-should-i-define-function-or-property-6786951da909)

I thought setting the Document.status from Client directly can violate the encapsulation. I reverted it based on your suggestion: 8febcf6

This reverts commit 9a2e440.

coderabbitai

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 286eca6 and 8febcf6.

Files selected for processing (1)

yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (1 hunks)

Files skipped from review as they are similar to previous changes (1)

yorkie/src/main/kotlin/dev/yorkie/core/Client.kt

7hong13 added 5 commits May 27, 2024 13:06

encapsulate Document

9a2e440

bump up libraries to latest

1d36f1d

code clean up

873edde

Add Tree concurrency tests

c079ade

fix lint

6bec809

7hong13 self-assigned this May 27, 2024

7hong13 changed the title ~~Tree concurrency tests~~ Add Tree concurrency tests May 27, 2024

7hong13 added 2 commits May 27, 2024 14:23

use H2_PRIOR_KNOWLEDGE

dcb49d4

increase timeout time

9f86bbf

7hong13 force-pushed the tree_concurrency_test branch from 75ad17e to 9f86bbf Compare May 27, 2024 07:41

7hong13 marked this pull request as ready for review May 27, 2024 09:23

7hong13 requested review from hackerwins and skhugh May 27, 2024 09:24

coderabbitai bot reviewed May 27, 2024

View reviewed changes

make document wait for its initialization

286eca6

coderabbitai bot reviewed May 28, 2024

View reviewed changes

skhugh reviewed May 28, 2024

View reviewed changes

skhugh approved these changes May 28, 2024

View reviewed changes

Revert "encapsulate Document"

8febcf6

This reverts commit 9a2e440.

coderabbitai bot reviewed May 28, 2024

View reviewed changes

7hong13 merged commit b552be7 into main May 28, 2024
4 of 6 checks passed

7hong13 deleted the tree_concurrency_test branch May 28, 2024 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tree concurrency tests #186

Add Tree concurrency tests #186

7hong13 commented May 27, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

7hong13 commented May 27, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

skhugh May 28, 2024

7hong13 May 28, 2024

skhugh May 28, 2024

7hong13 May 28, 2024

coderabbitai bot left a comment

Add Tree concurrency tests #186

Add Tree concurrency tests #186

Conversation

7hong13 commented May 27, 2024 • edited by coderabbitai bot Loading

What this PR does / why we need it?

Any background context you want to provide?

What are the relevant tickets?

Checklist

Summary by CodeRabbit

coderabbitai bot commented May 27, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

7hong13 commented May 27, 2024 • edited Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

skhugh May 28, 2024

Choose a reason for hiding this comment

7hong13 May 28, 2024

Choose a reason for hiding this comment

skhugh May 28, 2024

Choose a reason for hiding this comment

7hong13 May 28, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

7hong13 commented May 27, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

7hong13 commented May 27, 2024 •

edited

Loading